Skip to content

fix: bound ChainLock seen cache - #7424

Merged
PastaPastaPasta merged 2 commits into
dashpay:developfrom
thepastaclaw:fix/chainlock-seen-cache-bound
Jul 10, 2026
Merged

fix: bound ChainLock seen cache#7424
PastaPastaPasta merged 2 commits into
dashpay:developfrom
thepastaclaw:fix/chainlock-seen-cache-bound

Conversation

@thepastaclaw

@thepastaclaw thepastaclaw commented Jul 7, 2026

Copy link
Copy Markdown

Issue being fixed or feature implemented

Bounds ChainLock seen-CLSIG duplicate tracking so unvalidated or non-current CLSIG inventory cannot grow retained handler state without limit.

This improves defensive handling for peer-supplied ChainLock messages while preserving the existing best ChainLock validation and update flow.

What was done?

  • Replaced the unbounded seenChainLocks map with a bounded unordered_limitedmap.
  • Moved same-height/older ChainLock rejection before inserting into duplicate-tracking state.
  • Updated cleanup for the bounded cache.
  • Added focused unit coverage for stale CLSIG retention and cache bounding.

How Has This Been Tested?

Tested locally on macOS in a fresh worktree rebased on current upstream/develop:

  • make -C src test/test_dash -j$(sysctl -n hw.ncpu)
  • src/test/test_dash --run_test=llmq_chainlock_tests,chainlock_handler_tests
  • git diff --check upstream/develop..HEAD
  • COMMIT_RANGE=upstream/develop..HEAD test/lint/lint-whitespace.py
  • Scoped Codex review of upstream/develop..HEAD: no significant issues found; recommendation ship.

Breaking Changes

None.

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added or updated relevant unit/integration/functional/e2e tests
  • I have made corresponding changes to the documentation
  • I have assigned this pull request to a milestone (for repository code-owners and collaborators only)

@coderabbitai

coderabbitai Bot commented Jul 7, 2026

Copy link
Copy Markdown

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 76a0aa45-b9df-4b64-82a3-36053a682a42

📥 Commits

Reviewing files that changed from the base of the PR and between 59fafb0 and b5a7ba0.

📒 Files selected for processing (1)
  • src/test/llmq_chainlock_tests.cpp
💤 Files with no reviewable changes (1)
  • src/test/llmq_chainlock_tests.cpp

Walkthrough

This PR changes ChainlockHandler’s seenChainLocks container to a bounded unordered_limitedmap capped at MAX_SEEN_CHAINLOCKS. AlreadyHave checks the cache before falling back to m_chainlocks.GetChainLockByHash. Cache insertion and cleanup logic are updated, testing accessors expose cache limits, and tests cover duplicate suppression, cache bounds, and lookup after eviction.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Suggested reviewers: UdjinM6, PastaPastaPasta

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly matches the main change: bounding the ChainLock seen cache.
Description check ✅ Passed The description accurately summarizes the cache bound, duplicate-tracking changes, and tests added.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

github-actions Bot commented Jul 7, 2026

Copy link
Copy Markdown

✅ No Merge Conflicts Detected

This PR currently has no conflicts with other open PRs.

@thepastaclaw

thepastaclaw commented Jul 7, 2026

Copy link
Copy Markdown
Author

✅ Review complete (commit b5a7ba0)

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ff09613ed6

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread src/chainlock/handler.cpp Outdated

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

Source: reviewers claude/general=opus(failed), codex/general=gpt-5.5(completed), claude/dash-core-commit-history=opus(failed), codex/dash-core-commit-history=gpt-5.5(completed); verifier=gpt-5.5

The PR is focused and the main bounded-cache behavior matches its stated goal, including avoiding retention of stale same-height CLSIGs. I verified one in-scope regression: because the bounded cache is now the only source for AlreadyHave(), attacker-controlled invalid future CLSIGs can evict the current best CLSIG hash and make later inventory for an object the node already stores appear new.

🔴 0 blocking | 🟡 1 suggestion(s) | 💬 0 nitpick(s)

Verified Findings

  • [SUGGESTION] src/chainlock/handler.cpp:75-76: Check the stored best ChainLock when the seen cache misses
    AlreadyHave() now answers only from seenChainLocks, but that cache is intentionally evictable and future-height CLSIGs are inserted before signature validation. A peer can churn more than MAX_SEEN_CHAINLOCKS invalid higher-height CLSIGs, evict the hash for the current best ChainLock, and make later MSG_CLSIG inventory for that stored ChainLock look new. net_processing will then request the CLSIG even though Chainlocks::GetChainLockByHash() can already serve it. Falling back to the authoritative best-ChainLock store on a cache miss preserves the bounded duplicate cache without forgetting the one CLSIG the node actually retains.

Note: GitHub does not allow me to approve/request changes on my own PR, so this exact-SHA self-review is posted as a COMMENT while preserving the verified findings.

@thepastaclaw

Copy link
Copy Markdown
Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 7, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@thepastaclaw

Copy link
Copy Markdown
Author

Handled the ChainLock seen-cache follow-up in f807b437bd7.

What changed:

  • AlreadyHave(MSG_CLSIG, hash) now checks the bounded seen cache first, then falls back to the authoritative stored best ChainLock by hash.
  • Added a regression test that churns the bounded seen cache past capacity and verifies the retained best CLSIG is still treated as already-known.

Validation:

  • git diff --check
  • make -C src -j4 chainlock/libbitcoin_node_a-handler.o test/test_dash-llmq_chainlock_tests.o test/test_dash
  • ./src/test/test_dash --run_test=chainlock_handler_tests

@thepastaclaw

Copy link
Copy Markdown
Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 7, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

Carried-forward prior finding prior-1 is fixed: AlreadyHave() now falls back to Chainlocks::GetChainLockByHash() after a seen-cache miss, and the new regression test covers best-CLSIG availability after cache eviction. The only new latest-delta finding is commit-history hygiene: the second commit directly repairs behavior introduced by the first commit and should be folded before merge.

Source: reviewers: opus/claude general failed (extra-usage quota), gpt-5.5 codex general completed, opus/claude dash-core-commit-history failed (extra-usage quota), gpt-5.5 codex dash-core-commit-history completed; verifier: gpt-5.5 codex.

🟡 1 suggestion(s)

1 additional finding(s) omitted (not in diff).

🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `<commit:f807b437>`:
- [SUGGESTION] <commit:f807b437>:1: Fold the best-CLSIG repair into the bounded-cache commit
  The PR stack has `ff09613ed6c` adding the bounded ChainLock seen-cache behavior, followed by `f807b437bd7` fixing that same behavior by adding the authoritative best-CLSIG fallback and regression coverage. Dash PR hygiene expects each commit to be a complete logical change, and keeping these as separate commits records the first commit as an intermediate state where the bounded cache can forget the stored best CLSIG. Squash `f807b437bd7` into `ff09613ed6c`, or amend the original commit, so the history contains one complete `fix: bound ChainLock seen cache` change.

@thepastaclaw

Copy link
Copy Markdown
Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 7, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/chainlock/handler.cpp`:
- Around line 49-50: Update the seenChainLocks initialization in the chain lock
handler so the unordered_limitedmap uses a larger prune-after-size instead of
defaulting to MAX_SEEN_CHAINLOCKS. The issue is that the current constructor
only sets the max size, causing prune() to sort and evict on every insert once
full. Follow the pattern used by g_already_asked_for in net_processing.cpp: keep
the existing max size for seenChainLocks, but pass a larger second capacity to
batch evictions and reduce work while cs is held.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 8ecda1f5-0cb6-4a79-81d2-26a0dc048b4d

📥 Commits

Reviewing files that changed from the base of the PR and between f084efb and f807b437bd770a956e1f04611c1fbef3136f3847.

📒 Files selected for processing (3)
  • src/chainlock/handler.cpp
  • src/chainlock/handler.h
  • src/test/llmq_chainlock_tests.cpp

Comment thread src/chainlock/handler.cpp
@thepastaclaw
thepastaclaw force-pushed the fix/chainlock-seen-cache-bound branch from f807b43 to 67deff2 Compare July 8, 2026 00:17
@thepastaclaw

Copy link
Copy Markdown
Author

Resolved the commit-history feedback in 67deff2cd4f by folding the best-CLSIG fallback/regression coverage into the bounded ChainLock seen-cache commit. The new head is a single fix: bound ChainLock seen cache commit over upstream/develop, with no tree diff from the previously reviewed fixed head f807b437bd7.\n\nValidation:\n- ./autogen.sh && ./configure --without-gui\n- make -C src -j4 chainlock/libbitcoin_node_a-handler.o test/test_dash-llmq_chainlock_tests.o test/test_dash\n- ./src/test/test_dash --run_test=chainlock_handler_tests\n- git diff --check upstream/develop..HEAD\n- COMMIT_RANGE=upstream/develop..HEAD test/lint/lint-whitespace.py\n- Exact-head verifier rerun: fixed / no findings

@thepastaclaw

Copy link
Copy Markdown
Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 8, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

No carried-forward prior findings remain: prior-f807-commit-history is fixed because the current PR stack at 67deff2 is a single fix: bound ChainLock seen cache commit rather than a corrective follow-up commit. The latest delta from f807b437 is empty, and the cumulative ChainLock cache changes match the PR goal: stale CLSIGs are not retained, the seen cache is bounded, and AlreadyHave() falls back to the stored best ChainLock after cache eviction.

Source: reviewers claude/general=opus(failed), codex/general=gpt-5.5(ok), claude/dash-core-commit-history=opus(failed), codex/dash-core-commit-history=gpt-5.5(ok); verifier codex=gpt-5.5.

Note: posted as a COMMENT review because GitHub does not allow approving my own PR.

@thepastaclaw
thepastaclaw force-pushed the fix/chainlock-seen-cache-bound branch from 67deff2 to d0202da Compare July 8, 2026 01:49

Copy link
Copy Markdown
Member

Fixed the CI failure by keeping all tests in src/test/llmq_chainlock_tests.cpp under the required llmq_chainlock_tests suite name, while using per-case TestingSetup fixtures for the handler tests.

Validation:

  • test/lint/lint-tests.py
  • git diff --check
  • COMMIT_RANGE=upstream/develop..HEAD test/lint/lint-whitespace.py
  • make -C src test/test_dash-llmq_chainlock_tests.o test/test_dash -j4
  • src/test/test_dash --run_test=llmq_chainlock_tests

Copy link
Copy Markdown
Member

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 8, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/test/llmq_chainlock_tests.cpp`:
- Around line 196-207: The seen_chainlock_cache_is_bounded test only checks that
SeenChainLockCacheSizeForTesting() stays under max_size, so it can pass even if
ProcessNewChainLock never inserts anything. Update the test in
seen_chainlock_cache_is_bounded to verify the cache actually grows after the
first CreateChainLock/ProcessNewChainLock call, and ensure the handler is in an
active state (as in the other chainlock tests) before exercising the cache
bounds. Keep the existing bounded-size assertion, but make the test fail if
insertion does not happen.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 5b1c7dab-432d-4aee-a183-c2b66dfe5256

📥 Commits

Reviewing files that changed from the base of the PR and between 67deff2cd4f9246adf695fc564911eb5319ab546 and d0202da80858fbc08b257ef39a42d55b1f9b1108.

📒 Files selected for processing (3)
  • src/chainlock/handler.cpp
  • src/chainlock/handler.h
  • src/test/llmq_chainlock_tests.cpp
🚧 Files skipped from review as they are similar to previous changes (2)
  • src/chainlock/handler.h
  • src/chainlock/handler.cpp

Comment thread src/test/llmq_chainlock_tests.cpp
@thepastaclaw

Copy link
Copy Markdown
Author

Addressed CodeRabbit’s test-hardening finding in d0477eebec7 by activating the handler before the bounded-cache loop and asserting the first processed CLSIG actually grows seenChainLocks. This keeps the max-size assertion from passing vacuously.

Validation:

  • test/lint/lint-tests.py
  • git diff --check
  • COMMIT_RANGE=upstream/develop..HEAD test/lint/lint-whitespace.py
  • make -C src test/test_dash-llmq_chainlock_tests.o test/test_dash -j4
  • src/test/test_dash --run_test=llmq_chainlock_tests

@thepastaclaw

Copy link
Copy Markdown
Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 8, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

Source: reviewers claude/general=opus(failed: extra-usage quota), codex/general=gpt-5.5(completed), claude/dash-core-commit-history=opus(failed: extra-usage quota), codex/dash-core-commit-history=gpt-5.5(completed); verifier=codex gpt-5.5.

Carried-forward prior findings: none. The prior exact-head review at 67deff2c had no issues, and the latest delta to d0202da8 changes the test fixture scoping rather than reintroducing any earlier implementation or commit-history problem.

New findings in latest delta: one test-strength suggestion. Cumulative current-head validation still shows the bounded-cache behavior is sound, but the new bounded-cache test can pass even if the seen-cache insertion path stops being exercised. CodeRabbit's same-SHA top-level review raised the same concern; there was no inline comment id available for an automated reaction.

🟡 1 suggestion(s)

🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `src/test/llmq_chainlock_tests.cpp`:
- [SUGGESTION] src/test/llmq_chainlock_tests.cpp:201-205: Assert the bounded cache is actually populated
  This test only checks that the seen ChainLock cache size stays at or below the configured maximum. If `ProcessNewChainLock()` regressed to return before inserting every non-stale CLSIG into `seenChainLocks`, the cache would stay at size 0 and this test would still pass, so it would not prove the duplicate-tracking path that this PR keeps while bounding the cache. The test should verify that processed hashes are visible through `AlreadyHave()` before eviction and that the cache reaches the configured bound.

Comment thread src/test/llmq_chainlock_tests.cpp
@thepastaclaw

Copy link
Copy Markdown
Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 8, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

Source: reviewers claude/general=opus(failed: extra-usage quota), codex/general=gpt-5.5(completed), claude/dash-core-commit-history=opus(failed: extra-usage quota), codex/dash-core-commit-history=gpt-5.5(completed); verifier=codex gpt-5.5.

Prior finding reconciliation: the previous exact-SHA automated review at 47578dc9 had no verified findings.

Carried-forward prior findings: none.

New findings in latest delta 47578dc9..59fafb02: none. That delta is base movement from f084efb7 to cbab2549 in asset-lock functional-test files, not a new PR-authored ChainLock change.

Cumulative current-head review found no in-scope defects in the ChainLock seen-cache patch. CodeRabbit had no actionable inline findings, so no reactions were emitted.

Note: GitHub does not allow me to approve/request changes on my own PR, so this exact-SHA clean self-review is posted as a COMMENT.

@thepastaclaw

Copy link
Copy Markdown
Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 8, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

knst
knst previously approved these changes Jul 10, 2026

@knst knst left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm 59fafb0

Comment thread src/test/llmq_chainlock_tests.cpp

@knst knst left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM b5a7ba0

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

Prior reconciliation: NOT APPLICABLE — the exact-SHA artifact for 59fafb0 has an empty verified_output.findings array (confirmed by reading it), so there are no prior finding identities to carry forward. Carried-forward prior findings: none. New findings in latest delta 59fafb0..b5a7ba0 (a single include deletion in src/test/llmq_chainlock_tests.cpp): one verified commit-hygiene suggestion. Additional cumulative current-head findings: none — the bounded seen-CLSIG limitedmap, single-locked count-then-insert, erase-by-key cleanup, and AlreadyHave best-CLSIG fallback are correct and well covered.

Source: reviewers general=opus (completed), gpt-5.6-sol (failed: ACP refresh token revoked before review); dash-core-commit-history=opus (completed), gpt-5.6-sol (failed: ACP refresh token revoked before review); verifier=opus (completed).

🟡 1 suggestion(s)

1 additional finding(s) omitted (not in diff).

🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `<commit:b5a7ba0e>`:
- [SUGGESTION] <commit:b5a7ba0e>:1: Squash b5a7ba0e into 59fafb02 — it only removes an include the same PR added
  Commit 59fafb02 (`fix: bound ChainLock seen cache`) adds `#include <llmq/quorumsman.h>` to this test file, and the follow-up commit b5a7ba0e (`test: drop redundant quorum manager include`) removes that exact line, so the two commits net to zero in the cumulative diff. Verified: the include is genuinely redundant — `ProcessNewChainLock` takes `const llmq::CQuorumManager&` and the test passes `*m_node.llmq_ctx->qman`, which binds a same-type reference and does not require the complete type, so the removal is build-safe. Because this is a corrective fixup of code introduced earlier in the same PR, it carries no durable meaning in permanent `develop` history and only adds a bisect/blame stop with no standalone purpose. Recommend squashing b5a7ba0e into 59fafb02 (e.g. mark it `fixup!` and `git rebase --autosquash`) so the redundant include never enters history. Non-blocking: both commits build independently and the change is test-only, so bisect of the consensus-critical ChainLock production change is not endangered.

@thepastaclaw

Copy link
Copy Markdown
Author

The failing linux64_sqlite-build / Build source check is unrelated to this PR.

The build completed and then the pre-existing evo_dip3_activation_tests/verify_db_legacy test intermittently failed at src/test/evo_deterministicmns_tests.cpp:102 because GetTransaction() returned no transaction. This is the known asynchronous txindex race already addressed by #7266; I recorded this occurrence in #7443 with the failing job, root-cause evidence, and reproduction details.

This PR changes only the ChainLock handler/header and llmq_chainlock_tests.cpp, not the failing deterministic-masternode test. Its relevant ChainLock validation remains proper as-is, so no branch change, push, rebase, empty commit, or CI retrigger is warranted for this failure.

@thepastaclaw

Copy link
Copy Markdown
Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 10, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@kwvg kwvg left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK b5a7ba0

@PastaPastaPasta
PastaPastaPasta merged commit e002c28 into dashpay:develop Jul 10, 2026
85 of 86 checks passed
@UdjinM6 UdjinM6 added this to the 24 milestone Jul 11, 2026
thepastaclaw pushed a commit to thepastaclaw/dash that referenced this pull request Jul 22, 2026
b5a7ba0 test: drop redundant quorum manager include (PastaClaw)
59fafb0 fix: bound ChainLock seen cache (PastaClaw)

Pull request description:

  ## Issue being fixed or feature implemented

  Bounds ChainLock seen-CLSIG duplicate tracking so unvalidated or non-current CLSIG inventory cannot grow retained handler state without limit.

  This improves defensive handling for peer-supplied ChainLock messages while preserving the existing best ChainLock validation and update flow.

  ## What was done?

  - Replaced the unbounded `seenChainLocks` map with a bounded `unordered_limitedmap`.
  - Moved same-height/older ChainLock rejection before inserting into duplicate-tracking state.
  - Updated cleanup for the bounded cache.
  - Added focused unit coverage for stale CLSIG retention and cache bounding.

  ## How Has This Been Tested?

  Tested locally on macOS in a fresh worktree rebased on current `upstream/develop`:

  - `make -C src test/test_dash -j$(sysctl -n hw.ncpu)`
  - `src/test/test_dash --run_test=llmq_chainlock_tests,chainlock_handler_tests`
  - `git diff --check upstream/develop..HEAD`
  - `COMMIT_RANGE=upstream/develop..HEAD test/lint/lint-whitespace.py`
  - Scoped Codex review of `upstream/develop..HEAD`: no significant issues found; recommendation ship.

  ## Breaking Changes

  None.

  ## Checklist:

  - [x] I have performed a self-review of my own code
  - [x] I have commented my code, particularly in hard-to-understand areas
  - [x] I have added or updated relevant unit/integration/functional/e2e tests
  - [ ] I have made corresponding changes to the documentation
  - [ ] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_

ACKs for top commit:
  kwvg:
    utACK b5a7ba0

Tree-SHA512: 5626a69dfb1a1fbcbee56b62cc9fa442922a877f2d4ce33c51ade33f0718fe6cfae272e2136fb7e128e790edc8aacbbf694d0daae3a78406a98cb0543127d3ec
thepastaclaw pushed a commit to thepastaclaw/dash that referenced this pull request Jul 22, 2026
b5a7ba0 test: drop redundant quorum manager include (PastaClaw)
59fafb0 fix: bound ChainLock seen cache (PastaClaw)

Pull request description:

  ## Issue being fixed or feature implemented

  Bounds ChainLock seen-CLSIG duplicate tracking so unvalidated or non-current CLSIG inventory cannot grow retained handler state without limit.

  This improves defensive handling for peer-supplied ChainLock messages while preserving the existing best ChainLock validation and update flow.

  ## What was done?

  - Replaced the unbounded `seenChainLocks` map with a bounded `unordered_limitedmap`.
  - Moved same-height/older ChainLock rejection before inserting into duplicate-tracking state.
  - Updated cleanup for the bounded cache.
  - Added focused unit coverage for stale CLSIG retention and cache bounding.

  ## How Has This Been Tested?

  Tested locally on macOS in a fresh worktree rebased on current `upstream/develop`:

  - `make -C src test/test_dash -j$(sysctl -n hw.ncpu)`
  - `src/test/test_dash --run_test=llmq_chainlock_tests,chainlock_handler_tests`
  - `git diff --check upstream/develop..HEAD`
  - `COMMIT_RANGE=upstream/develop..HEAD test/lint/lint-whitespace.py`
  - Scoped Codex review of `upstream/develop..HEAD`: no significant issues found; recommendation ship.

  ## Breaking Changes

  None.

  ## Checklist:

  - [x] I have performed a self-review of my own code
  - [x] I have commented my code, particularly in hard-to-understand areas
  - [x] I have added or updated relevant unit/integration/functional/e2e tests
  - [ ] I have made corresponding changes to the documentation
  - [ ] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_

ACKs for top commit:
  kwvg:
    utACK b5a7ba0

Tree-SHA512: 5626a69dfb1a1fbcbee56b62cc9fa442922a877f2d4ce33c51ade33f0718fe6cfae272e2136fb7e128e790edc8aacbbf694d0daae3a78406a98cb0543127d3ec
PastaPastaPasta added a commit to PastaPastaPasta/dash that referenced this pull request Jul 29, 2026
b5a7ba0 test: drop redundant quorum manager include (PastaClaw)
59fafb0 fix: bound ChainLock seen cache (PastaClaw)

Pull request description:

  ## Issue being fixed or feature implemented

  Bounds ChainLock seen-CLSIG duplicate tracking so unvalidated or non-current CLSIG inventory cannot grow retained handler state without limit.

  This improves defensive handling for peer-supplied ChainLock messages while preserving the existing best ChainLock validation and update flow.

  ## What was done?

  - Replaced the unbounded `seenChainLocks` map with a bounded `unordered_limitedmap`.
  - Moved same-height/older ChainLock rejection before inserting into duplicate-tracking state.
  - Updated cleanup for the bounded cache.
  - Added focused unit coverage for stale CLSIG retention and cache bounding.

  ## How Has This Been Tested?

  Tested locally on macOS in a fresh worktree rebased on current `upstream/develop`:

  - `make -C src test/test_dash -j$(sysctl -n hw.ncpu)`
  - `src/test/test_dash --run_test=llmq_chainlock_tests,chainlock_handler_tests`
  - `git diff --check upstream/develop..HEAD`
  - `COMMIT_RANGE=upstream/develop..HEAD test/lint/lint-whitespace.py`
  - Scoped Codex review of `upstream/develop..HEAD`: no significant issues found; recommendation ship.

  ## Breaking Changes

  None.

  ## Checklist:

  - [x] I have performed a self-review of my own code
  - [x] I have commented my code, particularly in hard-to-understand areas
  - [x] I have added or updated relevant unit/integration/functional/e2e tests
  - [ ] I have made corresponding changes to the documentation
  - [ ] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_

ACKs for top commit:
  kwvg:
    utACK b5a7ba0

Tree-SHA512: 5626a69dfb1a1fbcbee56b62cc9fa442922a877f2d4ce33c51ade33f0718fe6cfae272e2136fb7e128e790edc8aacbbf694d0daae3a78406a98cb0543127d3ec
(cherry picked from commit e002c28)
PastaPastaPasta added a commit to PastaPastaPasta/dash that referenced this pull request Jul 29, 2026
b5a7ba0 test: drop redundant quorum manager include (PastaClaw)
59fafb0 fix: bound ChainLock seen cache (PastaClaw)

Pull request description:

  ## Issue being fixed or feature implemented

  Bounds ChainLock seen-CLSIG duplicate tracking so unvalidated or non-current CLSIG inventory cannot grow retained handler state without limit.

  This improves defensive handling for peer-supplied ChainLock messages while preserving the existing best ChainLock validation and update flow.

  ## What was done?

  - Replaced the unbounded `seenChainLocks` map with a bounded `unordered_limitedmap`.
  - Moved same-height/older ChainLock rejection before inserting into duplicate-tracking state.
  - Updated cleanup for the bounded cache.
  - Added focused unit coverage for stale CLSIG retention and cache bounding.

  ## How Has This Been Tested?

  Tested locally on macOS in a fresh worktree rebased on current `upstream/develop`:

  - `make -C src test/test_dash -j$(sysctl -n hw.ncpu)`
  - `src/test/test_dash --run_test=llmq_chainlock_tests,chainlock_handler_tests`
  - `git diff --check upstream/develop..HEAD`
  - `COMMIT_RANGE=upstream/develop..HEAD test/lint/lint-whitespace.py`
  - Scoped Codex review of `upstream/develop..HEAD`: no significant issues found; recommendation ship.

  ## Breaking Changes

  None.

  ## Checklist:

  - [x] I have performed a self-review of my own code
  - [x] I have commented my code, particularly in hard-to-understand areas
  - [x] I have added or updated relevant unit/integration/functional/e2e tests
  - [ ] I have made corresponding changes to the documentation
  - [ ] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_

ACKs for top commit:
  kwvg:
    utACK b5a7ba0

Tree-SHA512: 5626a69dfb1a1fbcbee56b62cc9fa442922a877f2d4ce33c51ade33f0718fe6cfae272e2136fb7e128e790edc8aacbbf694d0daae3a78406a98cb0543127d3ec
(cherry picked from commit e002c28)
PastaPastaPasta added a commit to PastaPastaPasta/dash that referenced this pull request Jul 29, 2026
b5a7ba0 test: drop redundant quorum manager include (PastaClaw)
59fafb0 fix: bound ChainLock seen cache (PastaClaw)

Pull request description:

  ## Issue being fixed or feature implemented

  Bounds ChainLock seen-CLSIG duplicate tracking so unvalidated or non-current CLSIG inventory cannot grow retained handler state without limit.

  This improves defensive handling for peer-supplied ChainLock messages while preserving the existing best ChainLock validation and update flow.

  ## What was done?

  - Replaced the unbounded `seenChainLocks` map with a bounded `unordered_limitedmap`.
  - Moved same-height/older ChainLock rejection before inserting into duplicate-tracking state.
  - Updated cleanup for the bounded cache.
  - Added focused unit coverage for stale CLSIG retention and cache bounding.

  ## How Has This Been Tested?

  Tested locally on macOS in a fresh worktree rebased on current `upstream/develop`:

  - `make -C src test/test_dash -j$(sysctl -n hw.ncpu)`
  - `src/test/test_dash --run_test=llmq_chainlock_tests,chainlock_handler_tests`
  - `git diff --check upstream/develop..HEAD`
  - `COMMIT_RANGE=upstream/develop..HEAD test/lint/lint-whitespace.py`
  - Scoped Codex review of `upstream/develop..HEAD`: no significant issues found; recommendation ship.

  ## Breaking Changes

  None.

  ## Checklist:

  - [x] I have performed a self-review of my own code
  - [x] I have commented my code, particularly in hard-to-understand areas
  - [x] I have added or updated relevant unit/integration/functional/e2e tests
  - [ ] I have made corresponding changes to the documentation
  - [ ] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_

ACKs for top commit:
  kwvg:
    utACK b5a7ba0

Tree-SHA512: 5626a69dfb1a1fbcbee56b62cc9fa442922a877f2d4ce33c51ade33f0718fe6cfae272e2136fb7e128e790edc8aacbbf694d0daae3a78406a98cb0543127d3ec
(cherry picked from commit e002c28)
PastaPastaPasta added a commit that referenced this pull request Jul 30, 2026
24920a0 chore: prepare v23.1.8 release (pasta)
2194248 Merge #7348: fix: penalize oversized notfound messages (pasta)
f5c72c3 Merge #7347: fix: punish invalid dstx messages (pasta)
550caf7 Merge #7465: fix(qt): handle pixel-sized fonts when scaling widgets (pasta)
e203710 Merge #7419: fix(net): bound CoinJoin message vector intake (pasta)
5f5b960 Merge #7418: fix(net): bound signing message vector intake (Pasta)
7cc2cca Merge #7450: test: make governance vote fixtures wire-valid (Pasta)
f011c80 Merge #7440: fix(net): bound governance vote signature deserialization (Pasta)
4b4d96a Merge #7442: fix(net): authorize governance inv responses via the net-layer per-peer request tracker (Pasta)
f855b13 Merge #7444: fix(net): bound bloom message vectors before allocation (Pasta)
5b5c6fb Merge #7415: fix: bound pending sig share queue (Pasta)
9bbe808 Merge #7416: fix(net): bound quorum data response vectors (Pasta)
da42f50 Merge #7424: fix: bound ChainLock seen cache (Pasta)
5b310df Merge #7438: fix: bound SPORK signature deserialization (Pasta)
e118d0c Merge #7259: fix: dangling point to cj client (Pasta)
9921621 Merge #7439: refactor: add bounded vector deserialization (Pasta)
89bdf7c Merge #7414: fix(net): throttle per-object governance vote sync requests (Pasta)
44c396d Merge #7402: fix: bound pending recovered sig queue to prevent remote OOM (Pasta)
05cfe27 Merge #7351: fix: limit signing share sessions per peer (pasta)
3ef3a5b Merge #7408: fix: bound DKG contribution blob intake (pasta)
0ea6532 Merge #7387: test: migrate governance inv cache coverage to unit tests (Pasta)
8ffdf7f Merge #7398: backport: compact block relay hardening (bitcoin#26898, bitcoin#27626, bitcoin#27743, bitcoin#26969, bitcoin#29412, bitcoin#32646, bitcoin#33296) (Pasta)
2915142 backport: bitcoin#27608 - p2p: Avoid prematurely clearing download state for other peers (PastaClaw)
90b5473 Merge #7396: fix: run of circular-dependencies with python3.15 (Pasta)
b003cdc Merge #7395: ci: update GitHub Actions pins for Node 24 (pasta)
97c3dd1 Merge #7394: fix: stabilize par help text in manpages (pasta)
8f8616b Merge #7372: backport: bitcoin#32693: depends: fix cmake compatibility error for freetype (pasta)
48f72be Merge #7360: fix: empty platformP2PPort deprecated field in protx listdiff results (pasta)
a8cccff Merge #7298: fix(qt): keep PoSe score visible when hiding banned masternodes (pasta)

Pull request description:

  Release PR for Dash Core v23.1.8, a patch release on top of v23.1.7.

  Fast-forwards from `v23.1.x` (currently at `chore: prepare v23.1.7 release`), 29 commits, no merge commits, no conflicts.

  ## Contents

  Backports of PRs already reviewed and merged on `develop`:

  `#7259` `#7347` `#7348` `#7351` `#7298` `#7360` `#7372` `#7387` `#7394` `#7395` `#7396` `#7398` `#7402` `#7408` `#7414` `#7415` `#7416` `#7418` `#7419` `#7424` `#7438` `#7439` `#7440` `#7442` `#7444` `#7450` `#7465`

  Plus `backport: bitcoin#27608`, a single commit taken from Dash #7237 because #7398's compact-block hardening depends on it. The rest of that v0.26 batch is intentionally not included on v23.1.x. The commit is byte-identical to its reviewed counterpart inside #7237.

  And release preparation: version bump, regenerated man pages, release notes, archived 23.1.7 notes.

  ## Note for reviewers: this branch was rebuilt

  An earlier revision of this PR was discarded and the branch rebuilt from scratch. Review comments on the previous revision point at commits that no longer exist, though the feedback itself was carried over (see below).

  The reason: several commits titled `Merge #NNNN` in the earlier revision contained substantial code that exists nowhere upstream — apparently written from a description of each PR rather than ported from its diff. For example, `feature_llmq_simplepose.py` is byte-identical between v23.1.7 and `develop`, yet the earlier `Merge #7408` rewrote 66 lines of it; `test/functional/p2p_governance_invs.py` does not exist on `develop` at all, yet had grown from 62 to 148 lines.

  That mislabeling matters because a commit titled `Merge #NNNN` invites less scrutiny, not more. It also had consequences: the earlier revision was **missing #7440 entirely**, and contained eleven consecutive commits that did not compile (code written against newer upstream APIs this branch does not have — `Misbehaving(Peer&)`, and `PeerIsBanned` used five commits before it was declared).

  Every commit on this branch has now been diffed against its upstream merge commit. Where a backport differs, it is because v23.1.x predates an upstream refactor and the change had to be applied to the pre-refactor file — for example #7418 and #7438 patch `signing_shares.cpp` / `spork.cpp` where upstream patches `net_signing.cpp` / `net_processing.cpp`.

  ## Dropped from this branch

  - **#7350** (`net: don't lock cs_main while reading blocks`) — dropped on review feedback. It is a 110-line lock-structure refactor of `ProcessGetBlockData` with no measured benefit, and it would add avoidable churn to the eventual master→develop merge-back. Nothing on this branch depends on it: #7398's compact-block work precedes it, and the remaining 14 commits replay with zero conflicts once it is removed. Thanks @knst.

  ## Added after the initial review pass

  - **#7351** (`fix: limit signing share sessions per peer`) — cherry-picked as a single
    commit and placed before #7402, matching upstream's merge order. The include block
    additionally carries `<ranges>`: upstream's diff adds only `<algorithm>` because develop
    already had it, whereas v23.1.x did not and the backported `GetSessionCount()` /
    `GetAnnouncementSessionCount()` use `std::ranges::count_if`.
  - **#7465** (`fix(qt): handle pixel-sized fonts when scaling widgets`) — cherry-picked from
    the five upstream commits. `optiontests.cpp` additionally includes `qt/guiutil_font.h`,
    because `fontsLoaded()` and `updateFonts()` are declared there on v23.1.x while develop
    declares them in `qt/guiutil.h`, which is all the upstream test includes.

  Two further backports were added later and applied without any adaptation --
  their diffs are byte-for-byte identical to upstream:

  - **#7347** (`fix: punish invalid dstx messages`)
  - **#7348** (`fix: penalize oversized notfound messages`)

  ## Adaptations worth flagging

  - **#7360** — upstream gates `platformP2PPort` / `platformHTTPPort` in `protx listdiff` behind `IsServiceDeprecatedRPCEnabled()`. On 23.x those deprecated fields are deliberately not enforced through gating (see `bbcd9d543e6`), so shipping the gate as-is would silently drop two fields that v23.1.7 always returned. Changed to `if (true)` with a comment, per review feedback, keeping the block aligned with `develop`. The substantive fix from #7360 — reading the live port from `netInfo` instead of the always-zero scalar — is retained.

  - **#7415** — the pending-map caps (`MAX_PENDING_SIG_SHARES_PER_NODE`, `MAX_PENDING_SIG_SHARES_TOTAL`) are backported. The additional bound upstream places on batches awaiting verification is not, because it guards a condition that does not exist here: upstream's dispatcher pushes one task per batch inside an inner loop, whereas v23.1.x pushes a single looping worker per 10 ms tick. There is no unbounded task queue to bound.

  - **Man pages** — regenerated without the `lock` debug category, which only exists under `DEBUG_LOCKCONTENTION` and so is absent from release binaries. Thanks @UdjinM6 for catching this.

  ## Known CI failure

  macOS jobs are expected to fail. `actions/upload-artifact@v6` rejects filenames containing `:`, and the Xcode SDK ships Perl man pages with `::` in the name. A release-branch-only workaround existed on the earlier revision but was dropped as it corresponds to no upstream PR. This is accepted for this release.

  ## Testing

  - Every commit through #7465 compiles individually (verified for 27 of the 29; the three additions below were verified at the tip) — verified individually, not just at the tip.
  - Full build clean; no new warnings.
  - Unit tests pass.
  - Functional tests pass: `feature_llmq_signing` (both variants), `feature_llmq_chainlocks`, `feature_llmq_dkgerrors`, `feature_llmq_is_cl_conflicts`, `p2p_instantsend`, `feature_dip3_deterministicmns` (both wallet types), `rpc_coinjoin`.
  - Qt unit tests pass (32 cases, run under the `cocoa` platform plugin so the pixel-sized
    font regression from #7465 actually executes rather than self-skipping).
  - Lint: one pre-existing `lint-cppcheck-dash` failure, identical on v23.1.7, in files this branch does not touch.

Top commit has no ACKs.

Tree-SHA512: 0fa469c9a33820aa85fbb8b90c5877409d09490f746f1300b05ceda470a600765f900bec42e5aad5d90a2d46b44e28c20e44dc4a8fe719553a072298069eaec4
@UdjinM6 UdjinM6 modified the milestones: 24, 23.1.8 Jul 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants